Fix form checkValidity(), remove vnode.dom === .activeElement from isFormAttribute() and setAttr() #2257

kevinkace · 2018-10-23T05:33:41Z

Remove vnode.dom === $doc.activeElement from isFormAttribute(), and setAttribute() to fix validityCheck(), #2256

Description

When using m.withAttr() to maintain an input value, the form's checkValidity() method sometimes returns true when form fields are not valid. This happens on the second and subsequent submissions.

Motivation and Context

This fixes checkValidation().

#2256

How Has This Been Tested?

Here's a version of the modified code on flems.io

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation change

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.* 1 test was failing before update
I have updated docs/change-log.md

fuzetsu · 2018-10-23T13:54:19Z

render/render.js

@@ -747,7 +747,7 @@ module.exports = function($window) {
 		}
 	}
 	function isFormAttribute(vnode, attr) {
-		return attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected" && vnode.dom === $doc.activeElement || vnode.tag === "option" && vnode.dom.parentNode === $doc.activeElement


Are we sure removing this activeElement check makes sense? It seems to be associated to changing the selected attr. Do we know what this check was trying to achieve? I don't think it's related to #2256
Here is the old condition with parentheses to clarify it.

return ( attr === 'value' || attr === 'checked' || attr === 'selectedIndex' || (attr === 'selected' && vnode.dom === $doc.activeElement) || (vnode.tag === 'option' && vnode.dom.parentNode === $doc.activeElement) );

dead-claudia

One nit below, but you also need to write some tests so we know the fix actually works.

dead-claudia · 2018-10-23T19:50:13Z

render/render.js

@@ -747,7 +747,7 @@ module.exports = function($window) {
 		}
 	}
 	function isFormAttribute(vnode, attr) {
-		return attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected" && vnode.dom === $doc.activeElement || vnode.tag === "option" && vnode.dom.parentNode === $doc.activeElement
+		return attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected" || vnode.tag === "option" && vnode.dom.parentNode === $doc.activeElement


Could you please revert this for now and only fix the one issue with input.value? The comment in the bug regarding this was me noting I wanted to investigate (since it appeared similar in use), not that I actively wanted to change it.

kevinkace · 2018-10-23T23:37:07Z

This is certainly wrong, but I'm having a heck of a time wrapping my head around ospec.

o("checkValidity", function() {
    var value = "";
    var spy = o.spy()
    var context = {
        handler: withAttr("value", spy)
    }
    var form = {tag: "form", attrs: {
        novalidate: true,
        onsubmit: function(e) {
            e.preventDefault();
        }
    }, children: [
        {tag: "input", attrs: {value: value, oninput: withAttr("value", function(v) { value = v; })}},
        {tag: "button", attrs: {type: "submit"}}
    ]}

    render(root, [form])

    context.handler({currentTarget: {value: "abcd"}})
    // submit form
    o(form.dom.checkValidity()).equals(false)
    context.handler({currentTarget: {value: "abc"}})
    // submit form
    o(form.dom.checkValidity()).equals(false)
})

https://codepen.io/kevinkace/pen/MPPOML

dead-claudia · 2018-10-25T17:38:20Z

@kevinkace

You don't need to rebuild anything - we have a bot that does it for us.
Have you taken a look at ospec's docs?

kevinkace · 2018-10-26T16:25:45Z

@isiahmeadows
I did look at the docs. I couldn't figure out how to make a test for this (submitting a form, using withAttr). Sadly I can't justify spending more time to learn this test framework.

I'm including the Fork in my project ATM.

barneycarroll · 2018-10-28T13:24:44Z

@isiahmeadows

you also need to write some tests so we know the fix actually works

I don't think this is fair, since this bugfix merely reverts bugfix code which wasn't tested for anyway.

Even if that weren't the case, extending Mithril's DOM mocks to emulate W3C form validation — let alone to the point that we can reasonably account subtle implementation details like this one — is a hiding to nothing.

I think having the back link to the test cases provided in the original issue is a good enough validation.

dead-claudia · 2018-10-28T19:34:57Z

@barneycarroll

I don't think this is fair, since this bugfix merely reverts bugfix code which wasn't tested for anyway.

Could you find the commit that added it, so we can find the surrounding context?

Even if that weren't the case, extending Mithril's DOM mocks to emulate W3C form validation — let alone to the point that we can reasonably account subtle implementation details like this one — is a hiding to nothing.

Or we could just invoke vnode.dom.focus() in a hook that runs before all DOM hooks (like onbeforeupdate), to trigger the behavior. It'd have the same effect, and it'd be a lot less work.

I think having the back link to the test cases provided in the original issue is a good enough validation.

I don't like adding bug fixes without tests, just in principle. I'd rather not us be back here a second time because of an erroneous "bug fix" that breaks it again - it's happened before already IIRC.

StephanHoyer · 2022-02-20T13:14:23Z

Already fix in next

Remove vnode.dom === .activeElement from isFormAttribute() and setAttr()

842e223

kevinkace requested a review from pygy as a code owner October 23, 2018 05:33

fuzetsu reviewed Oct 23, 2018

View reviewed changes

dead-claudia requested changes Oct 23, 2018

View reviewed changes

Revert removing vnode.dom === .activeElement in isFormAttr()

2cc3c18

Built assets

ae096c5

dead-claudia added Type: Bug For bugs and any other unexpected breakage Status: Needs test case For pull requests that lack one or more test cases for the feature or behavior in question labels Oct 25, 2018

dead-claudia force-pushed the next branch from 292a667 to 51e0aee Compare May 29, 2019 14:11

dead-claudia added a commit to dead-claudia/mithril.js that referenced this pull request Mar 16, 2020

Fix MithrilJS#2257

e76b169

dead-claudia added a commit to dead-claudia/mithril.js that referenced this pull request Mar 16, 2020

Fix MithrilJS#2257

455e511

dead-claudia added a commit to dead-claudia/mithril.js that referenced this pull request Mar 16, 2020

Fix MithrilJS#2257

4e317bc

StephanHoyer closed this Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix form checkValidity(), remove vnode.dom === .activeElement from isFormAttribute() and setAttr() #2257

Fix form checkValidity(), remove vnode.dom === .activeElement from isFormAttribute() and setAttr() #2257

kevinkace commented Oct 23, 2018

fuzetsu Oct 23, 2018

dead-claudia left a comment

dead-claudia Oct 23, 2018

kevinkace Oct 23, 2018

kevinkace commented Oct 23, 2018 •

edited

Loading

dead-claudia commented Oct 25, 2018

kevinkace commented Oct 26, 2018

barneycarroll commented Oct 28, 2018

dead-claudia commented Oct 28, 2018

StephanHoyer commented Feb 20, 2022

Fix form checkValidity(), remove vnode.dom === .activeElement from isFormAttribute() and setAttr() #2257

Fix form checkValidity(), remove vnode.dom === .activeElement from isFormAttribute() and setAttr() #2257

Conversation

kevinkace commented Oct 23, 2018

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

fuzetsu Oct 23, 2018

Choose a reason for hiding this comment

dead-claudia left a comment

Choose a reason for hiding this comment

dead-claudia Oct 23, 2018

Choose a reason for hiding this comment

kevinkace Oct 23, 2018

Choose a reason for hiding this comment

kevinkace commented Oct 23, 2018 • edited Loading

dead-claudia commented Oct 25, 2018

kevinkace commented Oct 26, 2018

barneycarroll commented Oct 28, 2018

dead-claudia commented Oct 28, 2018

StephanHoyer commented Feb 20, 2022

kevinkace commented Oct 23, 2018 •

edited

Loading